-
Notifications
You must be signed in to change notification settings - Fork 972
Strip cookies from requests fetch favicons #14286
Conversation
app/renderer/lib/faviconUtil.js
Outdated
wrapFaviconUrl: (url) => `${url}${suffix}`, | ||
unwrapFaviconUrl: (url) => url.substring(0, url.length - suffix.length), | ||
isWrappedFaviconUrl: (url) => url.endsWith(suffix) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: please move to js/lib
@diracdeltas added tests |
test/fixtures/cookie-favicon.html
Outdated
var host = window.location + '' | ||
var parts = host.split('/') | ||
parts.splice(-1, 1) | ||
host = parts.join('/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could just do var host = window.location.origin
test/unit/js/lib/faviconUtilTest.js
Outdated
require('../../braveUnit') | ||
|
||
describe('faviconUtil', function () { | ||
it('wraps, identifies and unwarps URLs', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: s/unwarp/unwrap
test/fixtures/cookie-favicon.html
Outdated
host = parts.join('/') | ||
|
||
function loadIco() { | ||
var faviconLocation = host.replace('127.0.0.1', 'localhost') + '/cookie-favicon.ico' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to git add cookie-favicon.ico?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some lint errors: https://travis-ci.org/brave/browser-laptop/jobs/386331308#L5472 |
@@ -1053,4 +1053,28 @@ describe('Bravery Panel', function () { | |||
}) | |||
}) | |||
}) | |||
|
|||
// see #14250 | |||
describe('Favicon requests cannot set cookies', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice test!
Fixes brave#14250 Auditors: @diracdeltas Test Plan: Manually navigate to https://www.grc.com/cookies/forensics.htm and observe no problems in the report Navigate to a website with a favicon (such as github.com) and observe the favicon rendering Unit tests: run the faviconUtil unit tests Webdriver test: Runs a test page that sets a 3rd party cookie and checks if the favicon request carries over the cookies
Weird, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed that the test page fails without this change and succeeds with it.
one issue is that Favicon requests cannot set cookies
test passes even without this change. we looked into it for a while and couldn't figure out why cookies were not being sent in the webdriver favicon request even when they are set.
master / 0.24.x: c08c815 |
Strip cookies from requests fetch favicons
Uplifted to 0.23.x with e104f5a |
Fixes #14250
Auditors: @diracdeltas
This is a quick and dirty fix for #14250. It involves some sketchy action on a distance and we can discuss what can be improved about it.
Test Plan:
Manually navigate to https://www.grc.com/cookies/forensics.htm and observe no problems in the report
Navigate to a website with a favicon (such as github.com) and observe the favicon rendering
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests